Skip to content

build_util: the per-script cap kills the process group, not just the child - #257

Merged
Jammy2211 merged 2 commits into
mainfrom
claude/backport-per-script-timeout-r3w1sv
Aug 23, 2026
Merged

build_util: the per-script cap kills the process group, not just the child#257
Jammy2211 merged 2 commits into
mainfrom
claude/backport-per-script-timeout-r3w1sv

Conversation

@Jammy2211

Copy link
Copy Markdown
Collaborator

Leg B of the per-script timeout backport (PyAutoMind backport_per_script_timeout.md). Merge this first — seven workspace runners import the helper it adds.

What

subprocess.run(timeout=...) kills only the direct child. A grandchild — a Popen'd helper, a multiprocessing worker, a JAX compile server — outlives the cap and runs on, holding whatever memory and GPU it had. Over a mega-run of hundreds of scripts those accumulate against every script that follows.

Adds run_capped, a drop-in for the subprocess.run(..., timeout=...) calls this module made: same TimeoutExpired and CalledProcessError, same captured output/stderr attributes, so every existing handler — _timeout_output, is_clean_skip_exit, the ScriptResult/TIMEOUT report paths — is untouched. It runs the child with start_new_session=True and SIGKILLs the group on expiry. kill_group is public so the workspace run_smoke.py runners import it rather than each keeping a copy.

Both execute_notebook and execute_script switch over.

This is a resource-leak fix, not a hang fix

Worth stating plainly, because an earlier draft of the Mind prompt claimed otherwise. On POSIX subprocess.run handles its own TimeoutExpired with process.kill() then process.wait() on the direct child only — it never re-communicates, so it cannot block on an inherited pipe. Measured: with a 3s cap it raised at 3.0s. What it leaves behind is the grandchild, measured at 1 surviving process running for its full lifetime.

The uncapped runners that genuinely hang to the Actions ceiling are a separate problem, in the workspace copies, not here.

Verification

  • tests/test_script_timeout.py — 27 passed.
  • Full suite — 354 passed, with the same 14 failures main already has in this environment (missing ipynb-py-convert and image optimisers; confirmed identical by re-running on a clean tree).
  • The new regression test asserts the grandchild is gone one second after the cap fires. Against the previous code it fails assert 1 == 0 with the TIMEOUT status already correct, which isolates the group kill as the thing under test rather than the timer.

Generated by Claude Code

James Nightingale and others added 2 commits August 23, 2026 18:31
…child

`subprocess.run(timeout=...)` kills only the direct child. A grandchild —
a Popen'd helper, a multiprocessing worker, a JAX compile server — outlives
the cap and runs on, holding whatever memory and GPU it had. Over a mega-run
of hundreds of scripts those accumulate against every script that follows.

Add `run_capped`, a drop-in for the `subprocess.run(..., timeout=...)` calls
this module made: same TimeoutExpired and CalledProcessError, same captured
output/stderr attributes, so every existing handler — `_timeout_output`,
`is_clean_skip_exit`, the ScriptResult/TIMEOUT report paths — is untouched.
It runs the child with `start_new_session=True` and SIGKILLs the group on
expiry. `kill_group` is public so the workspace `run_smoke.py` runners can
import it rather than each growing a copy.

Both `execute_notebook` and `execute_script` switch over. Note this is a
resource-leak fix, NOT a hang fix: on POSIX `subprocess.run` handles its own
timeout with `process.wait()` on the direct child only, so it already
returned at the cap. The uncapped runners that hang to the Actions ceiling
are a separate problem in the workspace copies, not here.

Regression test asserts the grandchild is gone one second after the cap
fires; against the previous code it fails `assert 1 == 0` with the TIMEOUT
status already correct, isolating the group kill as the thing under test.

Verified: tests/test_script_timeout.py 27 passed; full suite 354 passed with
the same 14 pre-existing failures main has (missing ipynb-py-convert and
image optimisers in this environment).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GTPM1RmMvSuMvJkEntAMv8
The docstring cited a satellite repo's issue by name. PyAutoHands is a
framework organ and must stay adoptable as a config-diff fork, so
repos_sync.py's tenant firewall rejects a new instance fact in organ code —
correctly, and growing the allowlist for a comment would be the wrong fix.

Rewritten to describe the mechanism rather than the incident: the leak this
module actually has (grandchildren outliving the cap) and, separately, the
uncapped-runner shape where the same group kill prevents a hang, noted as a
workspace-side bug rather than named.

No code change. Local check now passes:
  repos_sync.py --check --only "tenant firewall (organ code)" -> OK

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GTPM1RmMvSuMvJkEntAMv8
@Jammy2211
Jammy2211 merged commit bdd1aeb into main Aug 23, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant